Conversation
|
I don't think Pragmatically, however, I'm OK with merging this if you can do that and explain how this fix would unblock what you're actually working on. |
funsor/terms.py
Outdated
| (k, to_funsor(v, arg.inputs[k])) for k, v in subs if k in arg.inputs | ||
| (k, to_funsor(v, arg.inputs[k])) | ||
| for k, v in subs | ||
| if k in arg.inputs and k is not v |
I discovered this issue by examining In that test under Simple solution can be to check def eager_markov_product(sum_op, prod_op, trans, time, step, step_names):
if step:
result = sequential_sum_product(sum_op, prod_op, trans, time, dict(step))
...
return Subs(result, step_names) |
Yes, I believe this can be boiled down to much simpler failing test with just couple of lines of code than |
Yes, that would be very helpful! |
| with AdjointTape() as tape: | ||
| y = 2 * x | ||
| if use_subs: | ||
| y = y(i="i") |
There was a problem hiding this comment.
This substitution shouldn't change adjoint value.
| elif test == "other": | ||
| y = y(j=0) | ||
| elif test == "reduce": | ||
| y = funsor.terms.Reduce(ops.add, y, frozenset()) |
There was a problem hiding this comment.
In fact, it seems that any (unary) atomic op that doesn't change the arg doubles the adjoint value.
This proposes to avoid any trivial
subs(do not callinterpretif allsubsare trivial).Trivial subs can arise for example in eager
MarkovProductwithstep_names = {"prev": "prev", "curr": "curr"}:which then pollutes
AdjointTape.tapeunderadjointinterpretation (#493 #544).